Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove last dependency on pin-project #629

Closed
wants to merge 5 commits into from
Closed

Remove last dependency on pin-project #629

wants to merge 5 commits into from

Conversation

jplatte
Copy link
Contributor

@jplatte jplatte commented Jan 17, 2022

Followup to / depends on #603.

Was controversial and thus split out into its own PR (see #603 (comment) and following comments). The reason this can't be done using pin-project-lite is now documented as a code comment.

With a breaking-change coming up soon-ish, I think another alternative would be to replace this type with the one from the either crate and have them provide EitherProj plus the necessary unsafe code That would imply moving the Layer and Service implementations for Either out to tower-service & tower-layer but I think that should be fine. It would also imply a bunch of extra impls on Either that may or may not be nice to have, as well as an extra API change since either::Either has Left and Right variants rather than A and B.

cc @olix0r @davidpdrsn

@jplatte
Fix sometimes-unused dependencies
a508d86
@jplatte
Remove html_root_url
e280aef 
It is no longer recommended to set it:
rust-lang/api-guidelines#230
@jplatte
Remove unnecessary item from release instructions
9789c09 
Path dependencies are not problematic for publishing as long as a
version is also specified.
@jplatte
Remove pin-project dependency
a864d79 
The only thing it was generating is written out into the source instead.
@olix0r olix0r mentioned this pull request Jan 17, 2022
Merged
@olix0r
Copy link
Collaborator

olix0r commented Jan 17, 2022

I'm sympathetic to pin-project being a heavyweight dependency for such a simple use case.

On the other hand, I'm also conTcerned that this change will make tools like cargo-geiger flag tower.

Removing pin-project is probably worth it...

While we're here though: why does Either need to hold pinned values at all? Is this is just to implement Future? Doesn't the futures crate already ship a version of Either that implements Future? Perhaps we can remove this functionality completely in the next minor release?

@olix0r
Copy link
Collaborator

olix0r commented Jan 17, 2022

Also note that #550 tracks updating the Either type to have Left and Right variants, planned for the next minor release.

@davidpdrsn
Copy link
Member

My 2 cents: I think we should leave Either as it is currently and eat the dependency on pin-project. However for the next breaking version of tower I think we should change Either to not implement Future and change its response future to be a struct with a private inner enum that uses pin-project-lite.

While we're at it I also think we should change Either to not use BoxError as the error type but instead require the two inner services to have the same error type and use that. I've found it odd that combining two infallible services and gives you one that is fallible, since axum requires all services to be infallible.

@jplatte
Copy link
Contributor Author

jplatte commented Jan 18, 2022

While we're here though: why does Either need to hold pinned values at all? Is this is just to implement Future? Doesn't the futures crate already ship a version of Either that implements Future? Perhaps we can remove this functionality completely in the next minor release?

It's part of futures-util: https://docs.rs/futures-util/latest/futures_util/future/enum.Either.html

What I wrote about either::Either could of course be done with that one too. The impls in tower-service and tower-layer could even be added in a patch release, then other crates could build things off of that sooner.

While we're at it I also think we should change Either to not use BoxError as the error type but instead require the two inner services to have the same error type and use that. I've found it odd that combining two infallible services and gives you one that is fallible, since axum requires all services to be infallible.

👍 I've had this issue as well and had to roll my own option_layer because of it, including another separate Either type.

@jplatte jplatte closed this Jan 18, 2022
@olix0r olix0r mentioned this pull request Jan 18, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jplatte @olix0r @davidpdrsn